fix: cp-12.18.0 discard duplicate accounts on unlock #32621
Merged
+534
−321
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
✨ Files requiring CODEOWNER review ✨🧩 @MetaMask/extension-devs
📜 @MetaMask/policy-reviewers
🔗 @MetaMask/supply-chain
🖥️ @MetaMask/wallet-ux
|
Builds ready [a12ebda]
UI Startup Metrics (1231 ± 67 ms)
Benchmark value 1068 exceeds gate value 1061 for chrome browserify home mean domContentLoaded Benchmark value 339 exceeds gate value 334 for chrome webpack home p95 firstPaint Benchmark value 10 exceeds gate value 9 for firefox browserify home mean setupStore Benchmark value 1664 exceeds gate value 1660 for firefox webpack home p95 load Benchmark value 1664 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded Benchmark value 1641 exceeds gate value 1630 for firefox webpack home p95 loadScripts Sum of mean exceeds: 13ms | Sum of p95 exceeds: 24ms Sum of all benchmark exceeds: 37ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mikesposito
changed the title
@metamask/keyring-controller
4 tasks
Builds ready [7a8160e]
UI Startup Metrics (1196 ± 56 ms)
Benchmark value 175 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 12 exceeds gate value 11 for firefox browserify home mean getState Benchmark value 15 exceeds gate value 13 for firefox webpack home mean setupStore Sum of mean exceeds: 3ms | Sum of p95 exceeds: 115ms Sum of all benchmark exceeds: 118ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [ca12a89]
UI Startup Metrics (1208 ± 69 ms)
Benchmark value 1420 exceeds gate value 1405 for firefox browserify home mean uiStartup Benchmark value 1255 exceeds gate value 1245 for firefox browserify home mean load Benchmark value 1255 exceeds gate value 1239 for firefox browserify home mean domContentLoaded Benchmark value 26 exceeds gate value 25 for firefox browserify home mean firstReactRender Benchmark value 15 exceeds gate value 11 for firefox browserify home mean getState Benchmark value 1234 exceeds gate value 1230 for firefox browserify home mean loadScripts Benchmark value 1669 exceeds gate value 1660 for firefox browserify home p95 uiStartup Benchmark value 31 exceeds gate value 24 for firefox browserify home p95 getState Sum of mean exceeds: 50ms | Sum of p95 exceeds: 20ms Sum of all benchmark exceeds: 70ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [4cc86df]
UI Startup Metrics (1207 ± 63 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [14df23d]
UI Startup Metrics (1202 ± 58 ms)
Benchmark value 34 exceeds gate value 27 for firefox browserify home p95 setupStore Benchmark value 1957 exceeds gate value 1935 for firefox webpack home p95 uiStartup Benchmark value 1676 exceeds gate value 1660 for firefox webpack home p95 load Benchmark value 1675 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded Benchmark value 1654 exceeds gate value 1630 for firefox webpack home p95 loadScripts Sum of mean exceeds: 0ms | Sum of p95 exceeds: 89ms Sum of all benchmark exceeds: 89ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [5a74b5e]
UI Startup Metrics (1219 ± 63 ms)
Benchmark value 1956 exceeds gate value 1935 for firefox webpack home p95 uiStartup Benchmark value 1683 exceeds gate value 1660 for firefox webpack home p95 load Benchmark value 1681 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded Benchmark value 1662 exceeds gate value 1630 for firefox webpack home p95 loadScripts Sum of mean exceeds: 0ms | Sum of p95 exceeds: 102ms Sum of all benchmark exceeds: 102ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mikesposito
added a commit
to MetaMask/core
that referenced
this pull request
May 14, 2025
Dependent on: - #5725 ## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> It is no longer possible to persist duplicates in the vault, though users that already have duplicates will see them in the accounts list, and won't be able to do any action with their vault. These changes aim to discard duplicates, moving the keyring including a duplicate account to the unsupported array. Can be tested on extension with MetaMask/metamask-extension#32621 ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- THIS SECTION IS NO LONGER NEEDED. The process for updating changelogs has changed. Please consult the "Updating changelogs" section of the Contributing doc for more. --> ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Mark Stacey <[email protected]> Co-authored-by: Charly Chevalier <[email protected]>
naugtur
previously approved these changes
May 15, 2025
zone-live
previously approved these changes
May 15, 2025
OGPoyraz
previously approved these changes
May 15, 2025
matthewwalsh0
previously approved these changes
May 15, 2025
NidhiKJha
previously approved these changes
May 15, 2025
mikesposito
dismissed stale reviews from NidhiKJha, naugtur, zone-live, OGPoyraz, and matthewwalsh0
via
1764055
OGPoyraz
approved these changes
May 15, 2025
javiergarciavera
approved these changes
May 15, 2025
naugtur
approved these changes
May 15, 2025
NidhiKJha
approved these changes
May 15, 2025
Gudahtt
approved these changes
May 15, 2025
Builds ready [1764055]
UI Startup Metrics (1186 ± 56 ms)
Benchmark value 2203 exceeds gate value 2192 for chrome webpack home mean uiStartup Benchmark value 1735 exceeds gate value 1711 for chrome webpack home mean load Benchmark value 1727 exceeds gate value 1704 for chrome webpack home mean domContentLoaded Benchmark value 42 exceeds gate value 40 for chrome webpack home mean backgroundConnect Benchmark value 1722 exceeds gate value 1699 for chrome webpack home mean loadScripts Benchmark value 2514 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 2031 exceeds gate value 2030 for chrome webpack home p95 load Benchmark value 2010 exceeds gate value 2005 for chrome webpack home p95 domContentLoaded Benchmark value 235 exceeds gate value 90 for chrome webpack home p95 backgroundConnect Benchmark value 1987 exceeds gate value 1970 for chrome webpack home p95 loadScripts Sum of mean exceeds: 83ms | Sum of p95 exceeds: 233ms Sum of all benchmark exceeds: 316ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ccharly
approved these changes
May 15, 2025
There was a problem hiding this comment.
LGTM.
Regarding the discussion here about account.options.entropySource, here are some technical details and logs on why we will need to re-onboard the wallet (for internal users/QA):
- Solana accounts set the
entropySourcein theiraccount.options - We have no easy way to migrate those
account.optionsfor now, because theAccountsControllerre-create theInternalAccounts list based on keyring's accounts - Keyring's state (here, the Snap keyring), cannot be migrated, since it's part of the vault (encrypted)
- Thus making it impossible to update the
account.options.entropySourcewith the new generated entropy source ID
This means, that, after this PR gets merged, existing Solana accounts won't be usable (and they cannot be fixed). Also, we cannot remove Solana accounts anymore, leaving us with only one option, re-onboard the wallet.
This will make sure that the new Solana accounts will be using the proper entropySource, thus fixing the initial problem.
zone-live
approved these changes
May 15, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Updating
@metamask/keyring-controllerto^22.0.0:and
@metamask/accounts-controllerto^29.0.0:Related issues
Fixes: #32935
Manual testing steps
1. Add duplicate to the wallet state
1.1. Switch to the
v11.7.3branch, build locally, and install the extension fromchrome://extensions1.2. Onboard with an SRP of which we know the second account (
child guilt hollow arrive average popular nasty soon summer like scheme diary pill country rapid)1.3. Import an account that is part of the mnemonic (
0x80842b7e3cfb1118e86a427cdec418e3b4179ef5bbbfd71c02a76349831c8a8bwhich is the account at index 2 of the above SRP)1.4. Add a new account on the main HD
1.5. Switch to
Version-v12.17.1branch, and refresh the extension inchrome://extensions1.6. Unlock the wallet, you should see duplicates in your accounts list and you won't be able to add new accounts
2. Test the fix
2.1. Switch to this branch, build locally, and refresh the extension in
chrome://extensions2.2 Unlock the wallet, you shouldn't see duplicate accounts anymore, and you should be able to add new accounts
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist